Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

Add per realm stats #446

Closed
wants to merge 1 commit into from
Closed

Add per realm stats #446

wants to merge 1 commit into from

Conversation

jeremyfaller
Copy link
Contributor

@jeremyfaller jeremyfaller commented Sep 1, 2020 •
edited by icco
Loading

Adds per realm stats, and a simple visualization of the codes issued and
claimed for the past month.

Fixes #410

Release Note

Adds per-realm stats, and a simple visualization of the last 30 days of issued and claimed codes.

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeremyfaller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@googlebot googlebot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Sep 1, 2020
@google-cla google-cla bot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Sep 1, 2020
sethvargo
sethvargo reviewed Sep 1, 2020
View reviewed changes
cmd/server/assets/header.html Show resolved Hide resolved
pkg/controller/realmstats/index.go Outdated Show resolved Hide resolved
pkg/controller/realmstats/realmstats.go Outdated Show resolved Hide resolved
pkg/database/realm.go Outdated Show resolved Hide resolved
pkg/database/realm_stats.go Outdated Show resolved Hide resolved
pkg/database/token.go Outdated Show resolved Hide resolved
pkg/database/vercode.go Show resolved Hide resolved
sethvargo
sethvargo reviewed Sep 2, 2020
View reviewed changes
pkg/database/realm.go Outdated Show resolved Hide resolved
pkg/database/vercode.go Show resolved Hide resolved
@jeremyfaller
Copy link
Contributor Author

/hold

Hold on this right now. I don't like the error handling I've put in vercode.go. I'm going to add some more testing.

sethvargo
sethvargo reviewed Sep 2, 2020
View reviewed changes
pkg/database/vercode.go Outdated Show resolved Hide resolved
@jeremyfaller jeremyfaller force-pushed the issue_410 branch 2 times, most recently from 6e43da2 to 107f0e1 Compare September 2, 2020 19:28
mikehelmick
mikehelmick reviewed Sep 2, 2020
View reviewed changes
pkg/database/vercode.go Outdated Show resolved Hide resolved
mikehelmick
mikehelmick reviewed Sep 2, 2020
View reviewed changes
pkg/database/vercode.go Outdated Show resolved Hide resolved
@jeremyfaller
Copy link
Contributor Author

/hold

Holding AGAIN. while I work out what I don't understand about gorm.

@jeremyfaller
Copy link
Contributor Author

/unhold

mikehelmick
mikehelmick suggested changes Sep 3, 2020
View reviewed changes
pkg/database/token.go Outdated Show resolved Hide resolved
sethvargo
sethvargo reviewed Sep 3, 2020
View reviewed changes
pkg/database/vercode.go Outdated Show resolved Hide resolved
pkg/database/vercode.go Outdated Show resolved Hide resolved
@jeremyfaller
Add per realm stats
1be8b68 
Adds per realm stats, and a simple visualization of the codes issued and
claimed for the past month.
sethvargo
sethvargo reviewed Sep 7, 2020
View reviewed changes
pkg/database/vercode.go
scope.Log(fmt.Sprintf("failed to update stats: %v", err))
}
}

// Update the per-realm stats.
sql := `
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably guard this with a sanity:

if v.RealmID != 0 {

can be a follow-up

pkg/database/vercode.go
// per-realm stats. If any of that fails, an error code is returned.
func (v *VerificationCode) claim(db *Database, tx *gorm.DB, verCode string, realmID uint) error {
if expired, err := db.IsCodeExpired(v, verCode); err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to wrap this error like fmt.Errorf("failed to claim code: %w")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be a follow-up

pkg/database/vercode.go

// claim checks the VerificationCode, marks as claimed, and updates the
// per-realm stats. If any of that fails, an error code is returned.
func (v *VerificationCode) claim(db *Database, tx *gorm.DB, verCode string, realmID uint) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is a little weird to me now, and I'm not sure the comment matches. While this sets Claimed: true on the Verification Code, it does not save that code in the database. The caller still needs to call db.Save(v). That feels subtle and could lead to some bugs. Furthermore, we're updating the realm stats before successfully marking the code as claimed. I'm thinking this flow should be more like:

  1. Expiration checks
  2. Claimed check
  3. Mark as claimed on struct
  4. Save struct to database (stop if error)
  5. Update stats
  6. Return

Alternatively, we need to document this better. What do you think?

@sethvargo sethvargo mentioned this pull request Sep 10, 2020
Merged
@google google locked and limited conversation to collaborators Oct 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sethvargo sethvargo sethvargo left review comments

@mikehelmick mikehelmick mikehelmick requested changes

Assignees

@mikehelmick mikehelmick

Labels
cla: yes Auto: added by CLA bot when all committers have signed a CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Realm level stats
5 participants
@jeremyfaller @google-oss-robot @mikehelmick @sethvargo @googlebot